Repository navigation
feat: add a batch utility mode to admission control - #941
Conversation
8805eca to
de47901
Compare
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).
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).
61cb39b to
361cab2
Compare
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).
59f4177 to
682dd98
Compare
12c26bb to
7096c62
Compare
|
Rebased onto current master as a clean three-commit stack (post #927/#943/#952/#974 merge wave) and marked ready for review. Notable reconciliation choices: the extraction prompt adopts the landed hypothetical-grounding doctrine verbatim (register stretches, the about/within rule, unsure defaults to constructed); the deterministic constructed short-circuit now runs before any utility call in both the single-candidate path and the batch chunker, so an all-constructed chunk spends zero LLM calls. Prompt-contract tests keep every doctrine assertion and adapt only to the split system/user shape; the batch-embed test pins the batched-utility contract (profile candidates ride the Step-2 pre-compute batch). Full suite and all CI groups green. |
rwmjhb
left a comment
There was a problem hiding this comment.
Reviewed head 7096c62. The batching tests and full suite pass, but four new production behaviors need correction before merge:
-
openclaw.plugin.json advertises batchChunkSize as the chunk bound for every batch stage, but PluginConfig and parsePluginConfig omit it and the SmartExtractor construction never passes it. Admission utility also hardcodes 10, and the advertised consolidate stage has no implementation. Non-default values are silently ignored. Please wire the setting end to end for the supported stages, normalize its range, and narrow the schema text to behavior that actually exists.
-
scoreUtilityBatch falls back only when completeJson throws. The production LlmClient contract returns null for request, empty-response, and JSON failures, so the normal failure path is logged as a successful batch and every candidate silently receives utility 0.5. Treat null as a call-level failure and perform the documented standalone fallback, with a null-returning production-shaped test.
-
When embedBatch omits a vector, batch admission receives [], skips similarity search, and records novelty=1. processCandidate can later recover the individual embedding but reuses that stale admission result. Skip precomputed admission for missing vectors or recompute it after recovery, and add a partial-batch-vector regression.
-
This PR changes explicitly enabled standalone admission even when utilityMode remains standalone: every preset now defaults utilityVetoThreshold to 0.25, and utility scoring no longer receives the source conversation excerpt. A candidate that previously passed can now be rejected without opting into batching. Preserve the previous standalone default (for example veto=0 and bounded source context) or separate and document this policy migration with an explicit opt-in and compatibility tests.
|
All four items addressed in a1e1cb5.
Typecheck, build (dist committed), manifest verifier, and the full suite are green; all six new regressions fail on the previous head. |
|
Thanks for addressing the four review items in |
|
Rebased onto current master (post #934); the round history is squashed into one commit carrying the four review fixes. Conflict notes: index.ts resolved by keeping this branch's reviewed region and grafting master's new |
a1e1cb5 to
a3acf35
Compare
rwmjhb
left a comment
There was a problem hiding this comment.
Re-reviewed head a3acf35. The four requested production corrections are present on the rebased head: batchChunkSize is parsed and reaches the normal SmartExtractor admission path, null batch responses use standalone fallback, missing batch vectors are recomputed inline instead of reusing novelty=1, and the runtime utility-veto default is restored to 0. The targeted/full suites and GitHub CI are green.
A few follow-ups remain: the standalone fallback AdmissionController still uses chunk size 10, one mapped-reflection row failure can fail open the whole burst, the manifest still advertises veto default 0.25, and dedup prompts no longer include existing L1 overview details. These should be tightened, but none blocks the core opt-in batching path on this head.
Approving.
…ade) Document the pi host requirements the port inherits but cannot enforce: - defaultProvider/defaultModel in settings.json for embedded reflection/dreaming sub-agents - reachable embedding endpoint (e.g. Ollama) and valid LLM apiKey for smart extraction - re-run pi install after updating the repo (settings.json pins an absolute path) - known gap vs upstream: batch-utility admission mode (CortexReach#941) not ported Sync the section across all 11 README languages and update CHANGELOG.
Admission's utility scoring pays one LLM round trip per candidate. A reset with 10 mapped reflection rows fired 10 calls over roughly 24 seconds. This adds an opt-in
utilityMode: "batch"that scores a whole batch's utility with one call per chunk of up to 10 candidates, sharing the instructions and the conversation/source excerpt across the batch.Default stays
standalone; nothing changes for installs that don't opt in.Changes
src/admission-control.ts:AdmissionControlConfig.utilityModegains"batch"alongside"standalone" | "off".AdmissionController.evaluate()'s post-utility logic (confidence, novelty, recency, typePrior, the weighted decision, and audit construction) is extracted into a shared privateevaluateWithUtility(), used by both the existing single-candidate path and the new batch path. Only the utility feature's transport changes; everything else is unchanged.AdmissionController.evaluateBatch(items)scores a whole batch: one LLM call per chunk of up to 10 candidates, with the batch prompt listing every candidate indexed and demanding independent, absolute scoring per item (not a ranking within the batch), including a mixed-quality few-shot example. On a malformed or incomplete response (wrong count, missing/invalid index), it falls back to one standalone call per candidate in that chunk. This never fails the batch open or closed, it just costs more calls for that one chunk.{ system, user }even though this branch'sLlmClient.completeJson()still takes a single string (the two are concatenated here). This keeps the eventual merge with the system/user prompt-architecture split mechanical.src/smart-extractor.ts: whenadmissionControl.utilityMode === "batch",extractAndPersist()evaluates admission for every non-profile candidate in one upfrontevaluateBatch()call, instead of oneevaluate()call per candidate inside the sequential dedup/persist loop.processCandidate()accepts the precomputed evaluation and skips its own admission call when one is supplied. Profile candidates keep their existing separate admission check insidehandleProfileMerge, unaffected by batching. Standalone/off modes are untouched, since no precomputed evaluation is ever produced for them.openclaw.plugin.json:configSchema.properties.admissionControl.properties.utilityMode.enumgains"batch"(default stays"standalone").admissionControlhasadditionalProperties: false, so without this the new mode would fail config validation before ever reaching runtime code, unconfigurable as shipped.Tests (all new, TDD red-first)
test/admission-control-batch-utility.test.mjs: a batch of 3 candidates issues exactly one LLM call and produces three distinct audits; a malformed response falls back to one standalone call per candidate; batches larger than 10 chunk into multiple calls.test/smart-extractor-batch-admission.test.mjs: an extraction of 3 candidates inutilityMode: "batch"fires exactly oneadmission-utility-batchcall end to end.test/plugin-manifest-regression.mjs: asserts the schema declares"batch"in theutilityModeenum and that the default remains"standalone".Full local test chain green (
npm test, minus the pre-existing host-Ollama port conflict on 11434, unrelated to this change) andnpm run buildgreen.Batched dedup decider and merge writer (2026-07-17)
Extends the batch-call topology to the remaining two capture-path stages, so each pipeline event makes at most one LLM call per stage (extraction, admission utility, dedup decision, merge write):
dedup-decision-batchcall decides every admitted candidate that needs an LLM verdict (chunked pastDEDUP_BATCH_MAX_SIZEof 10). Each numbered candidate block carries its own retrieved-neighbor list; verdict vocabulary, thresholds,match_indexsemantics, and store operations are unchanged, only the call topology changes.merge-memory-batchcall writes every merge verdict, dedup-path and profile-path alike (chunked pastMERGE_BATCH_MAX_SIZEof 10). Candidates merging into the same target row are grouped into one job, which also removes the stale-content clobber that precomputed verdicts would otherwise allow when two candidates target one row.formatCandidateBlockmoved tosrc/extraction-prompts.tsso the admission prompts and the new batched dedup/merge prompts all render numbered candidate blocks through one shared formatter.dedup-decisionandmerge-memorypaths remain as the degraded fallback for candidates without a precomputed vector (batch pre-embed failure) and for directprocessCandidateuse without a merge queue.Two deliberate behavior notes: dedup vector searches now run before any merge writes land (previously a later candidate's dedup search could observe an earlier candidate's merge within the same extraction; batch-internal near-duplicate pruning already covers the common case), and a grouped same-target merge emits one persistence notification for its single store write instead of one per candidate.
Update: single-source prompt architecture (2026-07-17)
Extracted a shared
src/prompt-blocks.tsmodule and converged the admission, dedup, and merge-writer prompts onto it, closing the drift between the standalone and batch variants:You are a memory dedup judge.../You are a memory merge writer...).## Candidates/### N. category/ plainLabel: valuefield lines /#### Existing similar memoriesnested subsections, matching the extraction prompt's own markdown conventions. Every JSON output contract and few-shot example response is now fenced as a ```json code block.buildDedupPromptandbuildMergePrompt(the single-call variants) now take the full candidate object instead of separate positional strings, so they render through the same formatter as the batch variants.Known gap, intentionally out of scope for this PR: the grounding-awareness rule paragraph (currently only on the standalone admission prompt via a separate, unmerged branch) still cannot be composed into this branch's batch admission lane, because this branch's
CandidateMemorytype carries nogroundingfield and its admission prompt shows no conversation excerpt at all. That convergence needs the grounding-awareness branch merged first; flagging it here rather than fabricating a rule that references content this branch never shows the model.