fix: construct admission control independently of smart extraction#940
Draft
gorkem2020 wants to merge 5 commits into
Draft
fix: construct admission control independently of smart extraction#940gorkem2020 wants to merge 5 commits into
gorkem2020 wants to merge 5 commits into
Conversation
gorkem2020
force-pushed
the
fix/admission-without-smart-extraction
branch
from
July 15, 2026 21:02
f8e40ac to
2eceb34
Compare
gorkem2020
added a commit
to gorkem2020/memory-lancedb-pro
that referenced
this pull request
Jul 15, 2026
processCandidate's dedup-merge branch called handleMerge and then
unconditionally incremented stats.merged, even though handleMerge has two
paths that persist nothing: the existing row's getById read failing
(falls back to queuing the candidate as a new entry instead) and the
merge-memory LLM call returning null/unparseable (logs and returns with
the existing row untouched). handleProfileMerge's own call to handleMerge
had the identical bug one layer up — it always reported "merged" back to
processCandidate's profile branch regardless of what handleMerge actually
did.
handleMerge now returns which of its three real outcomes happened
("merged" | "created" | "llm-failed") instead of void, and both call
sites branch on it: a genuine merge counts as merged, the read-failure
fallback counts as created (a new entry really was queued), and an
LLM failure counts as neither — handleMerge already logs that case, so
this doesn't lose visibility, just stops claiming a merge happened when
nothing was written.
Picked this branch over fix/admission-without-smart-extraction (CortexReach#940) for
this fix: CortexReach#941 is already this batch's "truthful accounting" branch (item
3's malformed-batch-entry fix and item 6's transcript removal are both
about not claiming an LLM call did more than it did), so this fix extends
the same theme, while CortexReach#940's own scope (decoupling AdmissionController
construction from SmartExtractor) has no thematic overlap with merge
outcome accounting. Both branches carry the identical bug in identical
code; either would have worked mechanically.
Red-proved: both new failure-path tests failed against the prior
unconditional-increment code (stats.merged was 1 instead of 0 in both
cases) before the fix, and pass after. The happy-path test confirms
today's correct behavior stays correct (handleMerge's own two
store.update calls — merge content, then a best-effort support-stats
update — both still fire).
gorkem2020
added a commit
to gorkem2020/memory-lancedb-pro
that referenced
this pull request
Jul 17, 2026
processCandidate's dedup-merge branch called handleMerge and then
unconditionally incremented stats.merged, even though handleMerge has two
paths that persist nothing: the existing row's getById read failing
(falls back to queuing the candidate as a new entry instead) and the
merge-memory LLM call returning null/unparseable (logs and returns with
the existing row untouched). handleProfileMerge's own call to handleMerge
had the identical bug one layer up — it always reported "merged" back to
processCandidate's profile branch regardless of what handleMerge actually
did.
handleMerge now returns which of its three real outcomes happened
("merged" | "created" | "llm-failed") instead of void, and both call
sites branch on it: a genuine merge counts as merged, the read-failure
fallback counts as created (a new entry really was queued), and an
LLM failure counts as neither — handleMerge already logs that case, so
this doesn't lose visibility, just stops claiming a merge happened when
nothing was written.
Picked this branch over fix/admission-without-smart-extraction (CortexReach#940) for
this fix: CortexReach#941 is already this batch's "truthful accounting" branch (item
3's malformed-batch-entry fix and item 6's transcript removal are both
about not claiming an LLM call did more than it did), so this fix extends
the same theme, while CortexReach#940's own scope (decoupling AdmissionController
construction from SmartExtractor) has no thematic overlap with merge
outcome accounting. Both branches carry the identical bug in identical
code; either would have worked mechanically.
Red-proved: both new failure-path tests failed against the prior
unconditional-increment code (stats.merged was 1 instead of 0 in both
cases) before the fix, and pass after. The happy-path test confirms
today's correct behavior stays correct (handleMerge's own two
store.update calls — merge content, then a best-effort support-stats
update — both still fire).
gorkem2020
force-pushed
the
fix/admission-without-smart-extraction
branch
from
July 17, 2026 21:36
2eceb34 to
accc6d2
Compare
gorkem2020
added a commit
to gorkem2020/memory-lancedb-pro
that referenced
this pull request
Jul 18, 2026
processCandidate's dedup-merge branch called handleMerge and then
unconditionally incremented stats.merged, even though handleMerge has two
paths that persist nothing: the existing row's getById read failing
(falls back to queuing the candidate as a new entry instead) and the
merge-memory LLM call returning null/unparseable (logs and returns with
the existing row untouched). handleProfileMerge's own call to handleMerge
had the identical bug one layer up — it always reported "merged" back to
processCandidate's profile branch regardless of what handleMerge actually
did.
handleMerge now returns which of its three real outcomes happened
("merged" | "created" | "llm-failed") instead of void, and both call
sites branch on it: a genuine merge counts as merged, the read-failure
fallback counts as created (a new entry really was queued), and an
LLM failure counts as neither — handleMerge already logs that case, so
this doesn't lose visibility, just stops claiming a merge happened when
nothing was written.
Picked this branch over fix/admission-without-smart-extraction (CortexReach#940) for
this fix: CortexReach#941 is already this batch's "truthful accounting" branch (item
3's malformed-batch-entry fix and item 6's transcript removal are both
about not claiming an LLM call did more than it did), so this fix extends
the same theme, while CortexReach#940's own scope (decoupling AdmissionController
construction from SmartExtractor) has no thematic overlap with merge
outcome accounting. Both branches carry the identical bug in identical
code; either would have worked mechanically.
Red-proved: both new failure-path tests failed against the prior
unconditional-increment code (stats.merged was 1 instead of 0 in both
cases) before the fix, and pass after. The happy-path test confirms
today's correct behavior stays correct (handleMerge's own two
store.update calls — merge content, then a best-effort support-stats
update — both still fire).
createAdmissionController builds a controller from config alone, with no dependency on SmartExtractor. Availability depends only on admissionControl.enabled, laying the groundwork for gating write paths that don't go through extraction.
SmartExtractor no longer constructs its own AdmissionController from config; it now uses whatever controller instance it is handed (or none). This decouples admission gating from the extraction engine, so a controller built independently (e.g. via createAdmissionController) can be reused by write paths that don't go through SmartExtractor at all.
Construct the AdmissionController whenever admissionControl.enabled is true, not only when smartExtraction is also on. Previously the LLM client and controller were only ever built inside the smartExtraction block, so any write path other than SmartExtractor (reflection-mapped rows, the regex fallback) had no controller to borrow when smart extraction was disabled and silently stored ungated. The controller is now exposed on the plugin's singleton state alongside smartExtractor, so those other write paths can reference it directly once wired in, instead of reaching through smartExtractor. Registers the three new test files in package.json's test chain and scripts/ci-test-manifest.mjs (core-regression group).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
gorkem2020
force-pushed
the
fix/admission-without-smart-extraction
branch
from
July 18, 2026 15:51
accc6d2 to
7c097d7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Admission control could silently do nothing when
smartExtraction: false, even withadmissionControl.enabled: true. TheAdmissionControllerwas only ever constructed inside thesmartExtractioninit block, so any write path other thanSmartExtractorhad no controller to use and stored content ungated.This decouples admission-controller construction from the extraction engine choice, so its availability depends only on
admissionControl.enabled.Changes
src/admission-control.ts: addscreateAdmissionController(store, llm, config, debugLog), a factory that builds a controller purely from config, independent of any extractor.src/smart-extractor.ts:SmartExtractorno longer builds its ownAdmissionControllerfrom config. It now accepts a pre-built controller via a newadmissionControllerconstructor option and uses whatever it is given (or none).index.ts:smartExtraction !== falseoradmissionControl.enabled === true, instead of only the former.smartExtractor, so other write paths (for example a reflection-mapped-rows gate, or a regex-fallback gate) can reference it directly instead of reaching throughsmartExtractor.plugin registeredlog line with anadmissionControl: ON|OFFmarker, making the fix directly observable.Behavior
smartExtraction: false,admissionControl.enabled: true→ a controller is now constructed and available.smartExtraction: false,admissionControl.enabled: false→ still fully off, content still stored ungated. That is the configured meaning of "off," unchanged.smartExtraction: true→ decisions and audits are unchanged; the controller is built from the exact same store/llm/config/debugLog inputs it always was, just at a different call site.Tests (all new, TDD red-first)
test/admission-controller-standalone.test.mjs:createAdmissionControllerreturnsnullwhen disabled, and a working controller when enabled.test/smart-extractor-admission-controller-injection.test.mjs:SmartExtractorcalls whatever controller it is given, and skips gating entirely when none is configured (today's off-behavior, unchanged).test/admission-without-smart-extraction.test.mjs: registers the plugin withsmartExtraction: false+admissionControl.enabled: trueand asserts the new log marker; also covers the fully-off andsmartExtraction: truecases as regression guards.Full local test chain green (
npm test, minus the pre-existing host-Ollama port conflict on 11434, unrelated to this change) andnpm run buildgreen.