Route reflection mapped rows through the uniform dedup/merge pipeline - #988
Conversation
f595873 to
b1e754e
Compare
|
Marking ready for review: both overlapping PRs (#942, #964) have merged and this branch is rebased onto current master with the full gate set green (typecheck, fresh dist, manifest verifier, full suite). The change is production-proven on our deployment: the same form has run live since 2026-08-04 with the uniform pipeline handling organic reflection bursts (create, merge, skip, and utility-veto classes all observed in one burst on day one). |
|
Rebased onto master after #972's merge (registration union only). Full gates green. |
b1e754e to
1907ba6
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Requesting changes at 1907ba6. The happy-path tests and required CI checks are green, but the new external-lane entry point has several data-loss and provenance regressions:
-
persistGatedCandidatescreates a schema-incomplete synthetic admission audit atsrc/smart-extractor.ts:958and forwards it into merge/support handling.withAdmissionAuditthen replaces the target's completeadmission_controlrecord with onlydecisionandreason. Please represent "already gated" separately and do not persist this synthetic audit. -
The caller-provided reflection entry builder is consulted only by
buildStoreEntry. Matchedsupersede,contextualize, andcontradictpaths construct auto-capture entries directly (src/smart-extractor.ts:2734,:2907,:2983), losing the reflection event/session provenance, heading, mapped kind, decay settings, and admission audit. Build these rows from the external entry and then layer the verdict-specific metadata onto it. -
A
dedupPrefilterfailure atsrc/smart-extractor.ts:978is retried through inline dedup; if the same search fails again, the outer catch only logs and queues no create. That silently drops an already-admitted reflection row. This path should fail open through the caller's builder. -
flushPendingMergesdrops every queued addition when the merge response is failed or malformed (src/smart-extractor.ts:2544). These mapped rows were previously direct-stored, so unresolved external-lane additions should fall back to create instead of disappearing.
The new tests cover CREATE/MERGE/SKIP happy paths and judge failure, but not these branches. Please add regressions for all four. Also cover same-burst duplicates: unlike the normal extraction route, persistGatedCandidates omits batchDedup, so identical rows in one burst can both be created.
1907ba6 to
2e2f932
Compare
|
All five findings were correct, thank you for the careful read of the external-lane entry point. Fixed at
Five regression cells added, one per finding, in |
rwmjhb
left a comment
There was a problem hiding this comment.
Thanks for the latest hardening pass. The earlier processing/merge exception paths now fail open, but two data-integrity gaps remain:
-
A matched row disappearing between dedup and mutation still silently drops the admitted mapped row.
readMergeTargettreats a nullgetByIdresult as a valid empty target;applyMergedContentthen ignores both its own nullablegetByIdand the nullable result fromstore.update, after whichflushPendingMergesincrementsstats.mergedand emits a persistence notification. The SUPPORT path similarly returns when its target is absent, whileprocessCandidatestill incrementsstats.supported. Please make null reads/updates fall back to the caller-built CREATE row for externally gated candidates, report success only after a durable mutation, and add regressions for vanished MERGE and SUPPORT targets (including a nullupdateresult). -
The mapped-row audit parser does not match the production metadata shape.
index.tsstores the gate record as a JSON string undermetadata.admission_audit, butparseEntryAdmissionAuditreads only an object atmetadata.admission_control. The new test masks this by inventingadmission_control: { decision: "create", ... }; production emits anadmission_auditstring containing apass_to_deduprecord. Consequently MERGE and SUPPORT do not receive the actual mapped-row audit/provenance. Please parse the production field (including its nested JSON representation), or pass the original audit record directly, and replace the fixture with production-shaped metadata.
Non-blocking but worth correcting in the same area: the no-extractor fallback comment says a later burst will merge a duplicate stored during a search outage, but that path only performs the same pre-check and has no merge/reconciliation step.
The 10 focused uniform-pipeline tests pass and current GitHub CI is green. The local full-suite chain stalled in the unrelated existing update-consistency-lancedb.test.mjs; I am not treating that harness issue as an author blocker.
|
Both findings were correct — the vanished-target class was real data loss and the audit parser was reading a field production never writes. Fixed at
Four cells cover the round (vanished MERGE target at read, vanished during update via a null |
2e2f932 to
961cbab
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Thanks for fixing the production audit parsing and vanished-target fail-open paths; those previous blockers are resolved. The uniform route still has three correctness gaps:
-
Cross-category MERGE can make the target disappear from both smart-category views. The dedup prompt explicitly allows cross-category verdicts, but
applyMergedContentstampsmetadata.memory_categoryfrom the incoming candidate while itsstore.updatepatch leaves the target's legacycategorycolumn unchanged.MemoryStore.listfirst SQL-prefilters on that column and then validatesmetadata.memory_category. For example, merging apatternscandidate into apreferencerow leavescategory=preferenceandmemory_category=patterns: a patterns list never fetches it, while a preferences list fetches and then rejects it. Please preserve the target category on merge, or update both representations atomically, and add a regression that queries the merged row through the real category-filtered list path. -
The new same-burst guard drops rows before any category-aware or semantic decision.
persistGatedCandidatesappliesbatchDedupat cosine > 0.85 across all mapped rows, andbatchDedupalways discards the later item without considering category, mapped kind, exact text equality, or whether it adds information. The current test covers only identical pattern rows. A richer restatement or a related row from another reflection section can therefore be silently lost. Please restrict this shortcut to exact normalized duplicates, make it category-aware, or send non-identical pairs through the merge judge; add richer-row and cross-category regressions. -
A fail-open gate marker can overwrite a target's complete audit.
buildFailOpenResultproduces onlyprovenance/failedOpen/reason/error, butparseEntryAdmissionAuditcasts it toAdmissionAuditRecord. MERGE/SUPPORT then replace the target's completeadmission_controlwith that incomplete object, and admission statistics no longer see adecision. Please model fail-open evidence separately (or validate the union) and never replace a complete target audit with the marker.
Non-blocking follow-up: SUPPORT produces no reflection mdMirror entry, and MERGE is mirrored as generic smart-extraction, losing the reflection:<heading> provenance carried by the old path.
The 13 focused tests, full npm test, formal build/dist check, and GitHub CI all pass; none currently exercises these cases.
961cbab to
c6c34e3
Compare
|
All three were correct, fixed at
The non-blocking mirror note is also addressed for MERGE: batched merge jobs carrying an externally gated addition now notify with the caller's Red-proofed: exactly the four new cells fail at |
rwmjhb
left a comment
There was a problem hiding this comment.
Thanks for fixing the target-category merge, richer-restatement, and fail-open-audit cases. One same-burst data-integrity gap remains:
persistGatedCandidates builds the twin key from only candidate.category plus normalized text. Reflection lesson and decision rows both map to candidate category cases (and storage category fact), while retaining different mapped kinds, headings, importance, and decay policies. The reflection slicer emits Lessons before Decisions, so identical text under those two headings deterministically keeps the lesson and drops the durable decision before the dedup judge runs.
I reproduced this on c6c34e3 with one lesson and one decision carrying the same text: the result was created: 1, skipped: 1, and the only stored row was the lesson (importance=0.8, 7-day decay); the decision (importance=0.85, 45-day decay) was lost. The new "different reflection sections" test uses patterns versus preferences, so it verifies different categories but does not cover different mapped kinds sharing one category.
Please include the mapped kind or another caller-supplied lane identity in the exact-twin key, or otherwise collapse only exact duplicates from the same mapped lane, and add a lesson-versus-decision regression that proves neither row is silently discarded before semantic judging.
Also please replace the literal NUL delimiter currently embedded in src/smart-extractor.ts and dist/src/smart-extractor.js with a source escape or ordinary unambiguous delimiter. The actual 0x00 byte makes text tools such as rg classify these source files as binary.
Verification on the current head: the focused uniform-pipeline suite passes 17/17, npm run build succeeds and leaves generated files clean, the previously stalled LanceDB update test passes 6/6 standalone, and GitHub CI is green. The orchestrator's full-suite run itself timed out at 1200 seconds, so it did not verify the tests after that point.
c6c34e3 to
4259197
Compare
|
Both points fixed at
Full gates green (build, fresh dist, manifest verifier, focused suite 18/18, full suite exit 0). |
4259197 to
7cd18a4
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
The latest head resolves the previous lesson-versus-decision lane collision and removes the literal NUL delimiter. One same-burst semantic-dedup gap still blocks the uniform route:
persistGatedCandidates now removes only exact normalized twins within a lane, then runs every survivor through dedupPrefilter independently. That prefilter searches only already-persisted store rows. Because all prefilters and verdicts are computed before this burst's CREATE entries are stored, related non-identical candidates in the same burst never see each other when the store has no pre-existing neighbor; both short-circuit to CREATE without a semantic judge call.
I reproduced this on 7cd18a4 with the focused test's short lesson and richer restatement, an empty store, and identical vectors. The result was created: 2, skipped: 0, llmCalls: [], with both texts persisted. The existing test named "keeps a richer same-burst restatement alive for the judge" uses the same empty-store setup and makeLlm({}); its assertion that two rows are stored proves only that the exact-twin guard did not drop either row, not that either reached the judge.
Please include eligible same-lane burst candidates in one another's semantic dedup context, then add a regression that explicitly observes a dedup-decision-batch call and verifies a MERGE/SKIP verdict prevents two unconditional CREATEs. Keep negative controls for unrelated rows and distinct mapped lanes so the fix does not reintroduce the lesson/decision loss.
Verification on the current head: the focused uniform-pipeline suite passes 18/18, the orchestrator full suite passes, npm run build succeeds with a clean generated tree, and current GitHub CI is green. Those tests do not currently exercise same-burst semantic adjudication.
7cd18a4 to
cd4062b
Compare
|
Fixed at Same-lane siblings are part of each other's semantic dedup context. Earlier burst rows (same mapped kind and category, cosine at or above the store's own similarity threshold) join a candidate's neighbor list as virtual entries, so the empty-store short-circuit no longer bypasses the judge for related pairs; domain short-circuits like the preference-slot guard stay authoritative. A verdict against a sibling resolves after Your repro is the regression, both ways: the richer-restatement pair now produces one Red-proofed: exactly the two judge-observing cells fail against |
rwmjhb
left a comment
There was a problem hiding this comment.
The same-burst semantic-neighbor design now fixes the previous adjudication gap, and the MERGE/SKIP plus lane-separation regressions are good. One fail-open blocker remains in the deferred sibling phase:
After the initial bulkStore, persistGatedCandidates iterates pendingSiblingVerdicts. The SUPPORT branch directly awaits handleSupport, but handleSupport can throw from either store.getById or store.update. That loop has no per-verdict exception guard, so an exception rejects the whole persistence call instead of adding the admitted candidate to followupCreates. It also abandons any follow-up creates/merges already accumulated and skips every later deferred verdict because those queues are flushed only after the loop.
I reproduced this on cd4062b with two same-lane siblings, a SUPPORT verdict, and a throwing store.update: dedup-decision-batch ran, the call rejected with Error: support write outage, and only the first sibling remained stored. The admitted second row was neither used as support nor fail-open created.
Please make each deferred verdict independently fail open: catch getById/update and other deferred-resolution exceptions, enqueue the caller-built row exactly once, continue processing later verdicts, and still flush previously accumulated follow-up work. Add regressions for throwing getById and throwing update, plus a multi-verdict case proving one failure does not discard earlier or later follow-ups.
Non-blocking hardening: storedIdForSurviving falls back to matching only text when bulkStore returns fewer rows; identical text is intentionally allowed across lanes, so include lane/category identity in that fallback to avoid binding a deferred verdict to the wrong stored row.
Verification on the current head: the focused uniform-pipeline suite passes 20/20, the orchestrator full suite passes, npm run build succeeds with a clean generated tree, and current GitHub CI is green. None of those tests currently injects a deferred SUPPORT exception.
cd4062b to
c159242
Compare
|
Both points addressed at Deferred verdicts now degrade alone. Each entry in the deferred sibling loop resolves inside its own guard: a throwing Lane-aware stored-row fallback. Regressions (all red-first against the previous head): throwing |
|
Weekly status note: this PR has been ready for re-review since August 10. All four review rounds were addressed, each within hours of the feedback, checks are green, and there are no open threads. The re-review request to @rwmjhb has been pending since then. No rush intended, just keeping the thread current. Happy to address anything further whenever you have a moment. |
rwmjhb
left a comment
There was a problem hiding this comment.
The deferred sibling SUPPORT fixes are effective: throwing reads/writes now fail open per verdict, earlier follow-ups still flush, later verdicts continue, and lane-aware stored-row resolution addresses the previous blocker. Two data-integrity issues remain:
-
Supersede invalidation can reject after a partial commit.
persistGatedCandidatesfirst commits the superseding rows withbulkStoreAndValidate, then unguardedly awaitsapplyPendingSupersedeInvalidations. A read/update exception while invalidating the old row propagates before deferred sibling work runs. I reproduced this onc159242: the new superseding row was stored, the old row remained active, and the call rejected withError: invalidate outage. Please use an atomic supersede-and-store operation where available, or isolate each invalidation failure, continue later work, and downgrade the outcome to a plain CREATE rather than reporting it as superseded. Add throwing and null-returning invalidation regressions with later deferred work. -
MERGE and SUPPORT discard mutation-specific fail-open admission evidence. The production fail-open marker intentionally lacks
version/decision, soparseEntryAdmissionAuditreturns undefined; those verdicts do not store the caller-built entry carryingmetadata.admission_audit. In a merge reproduction, the resulting target metadata had noadmission_audit, no newadmission_control, and nofailedOpenmarker, even though unevaluated content became durable. Please model complete audits and fail-open evidence as a validated union or separate append-only field, preserving an existing complete target audit while recording that this mutation bypassed evaluation. Cover production-shaped MERGE and SUPPORT markers.
Non-blocking hardening: when CONTEXTUALIZE/CONTRADICT targets vanish, fall back to an ordinary CREATE without a relation to a nonexistent row.
Verification on the current head: the focused uniform-pipeline suite passes 24/24, the orchestrator full suite passes, npm run build succeeds with a clean generated tree, and current GitHub CI is green. These failure and provenance cases are not covered.
…reserve fail-open admission evidence on merge/support A throwing or nothing-written invalidation no longer rejects past the already-committed superseding row: each failure is isolated per row, later invalidations and deferred sibling verdicts continue, and the outcome downgrades to a plain create with the replacement's supersedes claim stripped. Production-shaped fail-open admission markers are parsed as evidence instead of being dropped: merge/support targets keep their complete admission_control and gain an append-only admission_bypass_events record proving the mutation carried unevaluated content. Contextualize and contradict verify their target still exists and fall back to an ordinary create without a dangling relation when it vanished.
|
All three items addressed in 7278eeb:
Regression coverage: throwing and nothing-written invalidations with later deferred sibling work continuing, plus production-shaped MERGE and SUPPORT fail-open markers asserting audit preservation and bypass evidence. All four fail on the pre-fix source and pass after; the focused suite is 28/28, the full chain and build are green, and the dist is rebuilt in the same commit. |
rwmjhb
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous round. I re-reviewed head 7278eeb: the focused reflection suite passes 28/28, the full suite and GitHub checks are green, and npm run build leaves the generated tree clean. The main isolation and admission-bypass fixes are directionally right, but there are still data-integrity blockers in the failure paths.
-
Deferred supersede invalidation can bind the old row to an unrelated create after
bulkStorefilters an earlier entry.handleSupersederecords a rawentryIndexfromcreateEntries, whileapplyPendingSupersedeInvalidationslater indexes the possibly shorter returned array (src/smart-extractor.ts:3268-3277,:3307).bulkStoreAndValidateexplicitly accepts a shorter result. I reproduced a three-row batch where row 1 was filtered, row 2 was the surviving replacement, and row 3 was an unrelated create: the old row'ssuperseded_bybecamenew-2(row 3) instead ofnew-1(the replacement). Resolve pending invalidations by stable entry identity, as the sibling-verdict path already does when positions shift, and do not invalidate unless the exact replacement is found. -
Grouped merges preserve admission evidence only from the first addition. Jobs group every candidate sharing a
matchId, butflushPendingMergespasses onlyjob.additions[0]?.admissionAuditatsrc/smart-extractor.ts:3009. A two-addition probe with a fail-open marker on the second row merged both texts but produced noadmission_bypass_eventsentry for that row. Aggregate every addition's complete audit/fail-open evidence into the capped append-only field. -
The target-disappearance hardening is still incomplete.
handleContextualizenow performs an unguardedgetByIdatsrc/smart-extractor.ts:3464; in the ordinary extraction loop the exception is only logged, so the admitted candidate is dropped instead of falling back to an unlinked CREATE. Separately,handleContradictignores a null result from the target update at:3545and still derivescontradictsfrom the earlier read; a null-update repro returned a new row with a dangling relation. Treat read/update failure or null astarget-missingand persist the candidate without a relation. -
A failed supersede is not fully reclassified as CREATE.
downgradeSupersedeToCreateignores a null cleanup result atsrc/smart-extractor.ts:3388, so a second write failure can leave the replacement'ssupersedesclaim durable while the old row remains active, despite the success-style downgrade log. Also,stats.supersededis incremented before deferred invalidation at:2339; with one failed and one successful invalidation my repro returned 2 instead of 1. Check the cleanup result, surface an unresolved repair state when stripping cannot be confirmed, and count only confirmed invalidations.
Please add regressions for the filtered-array identity shift, multi-addition bypass evidence, contextualize read failure, contradict null update, cleanup null, and confirmed-only supersede statistics.
…ped-merge evidence; guard contextualize/contradict fallbacks
|
All four addressed at c6ca9f9:
Six new regressions cover the requested scenarios (filtered-array identity shift in both directions, multi-addition bypass evidence, contextualize read failure, contradict null update, unresolved cleanup, confirmed-only statistics), red-proofed against the previous head: with the source change stashed, exactly those six fail and the prior 28 stay green. Focused file 34/34, full suite green, |
rwmjhb
left a comment
There was a problem hiding this comment.
Thanks for the comprehensive round. On head c6ca9f9, all six requested regressions are fixed: the focused suite passes 34/34, the full suite and GitHub checks are green, and the prior supersede/evidence/target-fallback blockers no longer reproduce. One combined partial-batch case still breaks the PR's core dedup guarantee:
-
Multiple deferred verdicts cannot reuse one surviving sibling anchor after
bulkStorefilters an unrelated row. In the shortened-result branch ofstoredIdForSurviving(src/smart-extractor.ts:1298-1328), the first lookup finds the anchor and adds its ID toclaimedIds. A later MERGE or SUPPORT with the samesiblingIndexthen excludes that same ID, resolves no target, and fails open to CREATE. The full-length branch correctly reuses the positional ID, so behavior changes only because an unrelated create was filtered. I reproduced this with one filtered row, one surviving anchor, and two SUPPORT verdicts: expectedstats.supported === 2, actual was1, with the second candidate falling back to a duplicate create. Cache the resolvedsurvivingIndex -> stored IDmapping; useclaimedIdsonly while assigning distinct surviving entries, then let every verdict for that index reuse the cached ID. -
Complete successful audits are being stored in the bypass-only history.
appendAdditionalAdmissionEvidenceatsrc/smart-extractor.ts:3909-3924appends every laterAdmissionWriteEvidence, including normalamac-v1pass audits, toadmission_bypass_events. That mislabels evaluated mutations and lets normal grouped merges consume the 20-record cap and evict genuine fail-open evidence. Keepadmission_bypass_eventsexclusive to fail-open markers and use a separately named typed history for additional complete audits.
Please add the combined filtered-result/shared-anchor regression. Non-blocking cleanup: interpretDedupVerdict already declares context_label, so the redundant (data as any) casts can be removed.
…lit complete audits out of the bypass history - storedIdForSurviving caches each surviving index's resolution so a second MERGE/SUPPORT verdict on the same sibling anchor reuses the row after bulkStore shortens the result; claimedIds only separates distinct surviving entries in the identity fallback. - appendAdditionalAdmissionEvidence keeps admission_bypass_events exclusive to fail-open markers; additional complete audits append to the new capped admission_control_history field so pass audits can never evict genuine bypass evidence. - interpretDedupVerdict drops the redundant casts around context_label. Regressions: the combined filtered-result/shared-anchor case (two supports on one surviving anchor with an unrelated row filtered) and a grouped-merge evidence-routing case; both red-proofed against the unfixed source.
|
Both findings fixed on head dc49f36.
Also removed the redundant Both new tests were red-proofed against the unfixed source (each fails exactly as the review describes: |
rwmjhb
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous round. On head dc49f36, the cached shared-anchor mapping and split audit histories fix the two prior blockers. Two same-burst paths still break the uniform dedup behavior:
-
Deferred verdict chains do not resolve transitively.
createSlotBySurvivingis populated only when a survivor directly queues a CREATE. If B merges into A, B remains a virtual sibling but has no create slot; when C then merges into B,storedIdForSurviving(B)returns undefined and C falls open to CREATE. I reproduced this in an empty store withB MERGE AandC MERGE B: the result wascreated: 2, merged: 1, with both A and C persisted despite both semantic merge verdicts. Please track each survivor's final durable anchor transitively, including chains that end at an existing row, and add regressions for A-created/B-merges-A/C-merges-B plus an anchor that itself merges into a stored row. -
The preference-slot short circuit suppresses eligible sibling adjudication.
persistGatedCandidatesyields to burst siblings only for the exactNo similar memories foundshort circuit. When a stored preference is a different item from the same brand, the preference guard returns CREATE and related same-lane siblings never reach the batch judge. With an existingfries from McDonald'srow and two reworded burger candidates, both burger rows were created and the dedup judge was never called. When the guard excludes stored neighbors but eligible burst siblings exist, please adjudicate those siblings alone and add this same-brand/different-stored-item/same-incoming-item regression.
Current split GitHub CI checks are green. The local serial npm test run hit the orchestrator's 180-second harness limit after all visible tests had passed, so I am not treating that timeout as an assertion failure. The two behavior reproductions above are the blockers.
…ate short-circuits yield to burst siblings - Survivors that merged/supported/skipped into an earlier sibling now record a sibling anchor, and ones whose verdict targeted an existing stored row record that row; storedIdForSurviving chases the chain transitively (indices strictly decrease) so B-merges-A / C-merges-B collapses into A's row, and chains ending at a stored row resolve there instead of falling open to a duplicate create. - A create-decision dedup short-circuit (nothing stored, or the preference-slot guard) is authoritative about stored rows only: eligible burst siblings now still reach the batch judge, alone, so same-item rewordings cannot double-create behind the guard. Regressions (red-proofed): transitive merge chain, sibling verdict through an anchor merged into a stored row, and preference-guard sibling adjudication.
|
Both round-8 blockers fixed on head 8276926.
All three new tests were red-proofed against the unfixed source (the chain case reproduces exactly your |
rwmjhb
left a comment
There was a problem hiding this comment.
Thanks for fixing the previous transitive-MERGE and preference-slot cases. On head 8276926, the 39 focused tests and full suite pass, npm run build succeeds, and rebuilding leaves the worktree clean. One sibling-chain variant still breaks the durable-anchor guarantee:
A SKIP anchor is discarded before a later sibling can resolve through it. interpretDedupVerdict only includes matchId for merge/support/contextualize/contradict/supersede, not for skip (src/smart-extractor.ts:2720-2724). The new bookkeeping records anchorSiblingBySurviving for a SKIP only when pre.matchId is present (:1222-1231). As a result, when B is judged SKIP against A and C is judged MERGE against B, B has no recorded path to A; deferred resolution treats B as having no persisted row and fail-opens C to CREATE.
I reproduced this with three same-lane paraphrases in an empty store. The batch judge returned B SKIP A and C MERGE B; the result was created: 2, merged: 0, skipped: 1, with both A and C durable and no merge-writer call. The existing regression covers B MERGE A -> C MERGE B, so it does not exercise this path.
Please preserve the valid sibling target for SKIP verdicts (or otherwise record the skipped survivor's durable anchor), and add regressions for B SKIP A -> C MERGE B and the analogous SUPPORT-through-skipped-anchor case.
…durable anchor interpretDedupVerdict now includes skip in the matchId decisions: a same-burst SKIP records its sibling anchor (and a stored-row SKIP its row anchor), so later verdicts chaining through the skipped survivor resolve to the durable row instead of failing open to a duplicate create. The skip handler itself still ignores the target. Regressions (red-proofed): B-SKIP-A with C-MERGE-B, and the analogous SUPPORT-through-skipped-anchor case.
|
Fixed on head shown below: Both requested regressions are in and were red-proofed against the unfixed source (each reproduces the fail-open duplicate exactly as described). Focused suite 41/41; full suite, typecheck, and build green with a clean worktree after rebuild. |
… LanceDB backend)
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head a0d556b (functional fix at d7a10c9). The previous SKIP-anchor blocker is resolved: the focused suite now includes both B SKIP A -> C MERGE B and B SKIP A -> C SUPPORT B, and all 41 mapped-row pipeline regressions pass. The full test suite passes, npm run build succeeds, generated output remains clean, and current required GitHub checks are green.
I found no remaining merge blocker. The fail-open behavior during sustained dedup-judge outages and the added serial LLM work inside the reflection lock remain operational tradeoffs, but they are bounded and do not invalidate this fix.
What
Routes reflection mapped rows (User model deltas, Agent model deltas, Lessons, Decisions) through the same dedup/merge pipeline extraction candidates use, whenever smart extraction is enabled.
SmartExtractor.persistGatedCandidates(items, options): an entry point for candidates whose extraction and admission already happened in another lane. From there they take exactly the extraction candidates' path: batched dedup decider, verdict handling, batched merge writer, bulk create. A duplicate mapped row now MERGES into its existing target instead of landing beside it.WeakMapkeyed by candidate object identity), so a CREATE verdict persists the caller's entry with reflection metadata intact, while merge/skip verdicts operate on existing rows through the shared machinery. Extraction's own candidates can never collide with an external lane's builders.gateMappedReflectionEntries) is unchanged and still runs first; a syntheticpass_to_dedupevaluation tellsprocessCandidatenot to score admitted rows a second time.bulkStorepath, strictly exclusive with the new route. Its near-duplicate pre-check is kept but downgraded from fail-closed to fail-open: previously a vector-search failure silently dropped the row; now the row stores, and the worst case is a duplicate a later burst merges.Why
#931 declared this debt explicitly ("deliberately not reused here yet"): mapped rows got a single 0.95 vector-similarity cutoff as their only dedup, with no LLM-mediated merge/contextualize decision, so reworded repeats accumulate as near-duplicate rows over time. With the batch subsystem from #941 on master, the uniform route costs one batched dedup call per admitted burst (chunked past 10) and zero calls when nothing similar exists.
Notes
_reflectionHeadingand friends) survives on CREATE verdicts; content merged into an existing row adopts that row's metadata. Importance on non-create verdicts comes from the existing row.bulkStoredirectly; the no-extractor fallback keeps its historical shape). All five new cells fail on master before this change.Live evidence
Deployed on a downstream install and exercised with a real reflection run over a day-long session. The burst produced
7 created, 2 merged, 4 skipped: the admission gate independently rejected one row before the pipeline (utility veto), and both merges landed on their existing target rows instead of creating duplicates beside them. No errors across the plugin restart or the run.