feat(recall): cap injected memory context#71
Conversation
Signed-off-by: 李冠辰 <liguanchen@xiaomi.com>
Signed-off-by: 李冠辰 <liguanchen@xiaomi.com>
|
感谢您如此及时地解决了 #70 的问题!我们会内部评估,有结论后会尽快反馈。 |
- Move preExtractMemories to newMessages only (after background/new split) to prevent extracting memories from background context that should only serve as conversational context for the LLM - Remove MEDIUM-confidence hints logging (hints not wired to LLM prompt; keeping types as interface for follow-up PR) - Remove src/ from package.json files field to fix Size Guard limit (matches pattern from Tencent#76 and Tencent#71) - Export callLlmExtraction and passesConfidenceCheck for testability - Add pre-extractor.test.ts covering: - Background messages not pre-extracted - HIGH-confidence dedup via mergeExtractedMemories - Malformed JSON triggers exactly one retry - Confidence filtering does not reject valid persona/instruction
There was a problem hiding this comment.
DO NOT modify this file, which might cause some problems. We have increased the limitation of Size Guard from 512KB to 2048KB.
There was a problem hiding this comment.
Resolved in 8031ec0: restored package.json to match upstream/main and removed the manifest / extension entry changes from this PR. Fresh checks are green, including Size Guard under the updated 2048KB limit.
|
Thanks for the review. I restored Fresh local verification after the update:
|
| recall: { | ||
| enabled: bool(recallGroup, "enabled") ?? true, | ||
| maxResults: num(recallGroup, "maxResults") ?? 5, | ||
| maxCharsPerMemory: normalizeNonNegativeInt(num(recallGroup, "maxCharsPerMemory"), 800), |
There was a problem hiding this comment.
- normalizeNonNegativeInt() is unnecessary here.
All other config fields (maxResults, scoreThreshold, timeoutMs) use the simple num(...) ?? defaultValue pattern — these two fields don't need special treatment.
Defensive handling for negative/non-integer values is already done on the consumer side in auto-recall.ts's normalizeBudgetLimit() (<= 0 → treated as disabled, Math.floor() for rounding). No need to duplicate that at the parse layer.
- The defaults should be 0 (disabled), not 800 / 3000.
This is a new feature — the default behavior should remain backward-compatible. Existing users upgrading should not suddenly see their recall results being truncated or dropped. Those who need this guard can opt in by explicitly setting the values.
There was a problem hiding this comment.
Resolved in c588558: recall budget config now follows the existing num(...) ?? 0 pattern, defaults are disabled, and normalizeNonNegativeInt() was removed.
| "properties": { | ||
| "enabled": { "type": "boolean", "default": true, "description": "是否启用自动召回" }, | ||
| "maxResults": { "type": "number", "default": 5, "description": "召回最大结果数" }, | ||
| "maxCharsPerMemory": { "type": "number", "default": 800, "description": "单条 L1 记忆注入的最大字符数;填 0 表示不限制" }, |
There was a problem hiding this comment.
Resolved in c588558: plugin schema default is now 0.
| | `storeBackend` | `"sqlite"` | Storage backend: `sqlite` | | ||
| | `recall.strategy` | `"hybrid"` | Recall strategy: `keyword` / `embedding` / `hybrid` (RRF fusion, recommended) | | ||
| | `recall.maxResults` | `5` | Number of items returned per recall | | ||
| | `recall.maxCharsPerMemory` | `800` | Max characters injected for one recalled L1 memory; `0` disables this guard | |
There was a problem hiding this comment.
Resolved in c588558: README default is now 0.
| | `storeBackend` | `"sqlite"` | 存储后端:`sqlite` | | ||
| | `recall.strategy` | `"hybrid"` | 召回策略:`keyword` / `embedding` / `hybrid`(RRF 融合,推荐) | | ||
| | `recall.maxResults` | `5` | 每次召回条数 | | ||
| | `recall.maxCharsPerMemory` | `800` | 单条 L1 记忆注入的最大字符数;`0` 表示不限制 | |
There was a problem hiding this comment.
Resolved in c588558: README_CN default is now 0.
There was a problem hiding this comment.
There are no existing test files under src/core/hooks/. The budget logic in normalizeBudgetLimit / truncateRecallLine is straightforward enough that the config parsing coverage in src/config.test.ts plus manual verification should suffice for this PR. Let's remove this file for now.
There was a problem hiding this comment.
Resolved in c588558: removed src/core/hooks/auto-recall.test.ts from this PR.
There was a problem hiding this comment.
Resolved in c588558: removed src/config.test.ts from this PR.
| continue; | ||
| } | ||
|
|
||
| const separatorChars = budgeted.length > 0 ? 1 : 0; |
There was a problem hiding this comment.
The 1 here implicitly assumes the join separator is "\n" (line 212). Consider adding a brief comment like // "\n".length to make the coupling explicit
There was a problem hiding this comment.
Resolved in c588558: introduced RECALL_LINE_SEPARATOR and use its .length for budget accounting, matching the final join separator.
| const totalBounded = truncateRecallLine(perMemoryBounded, remainingChars); | ||
| budgeted.push(totalBounded); | ||
| usedChars += separatorChars + totalBounded.length; | ||
| if (totalBounded !== perMemoryBounded) truncatedCount++; |
There was a problem hiding this comment.
truncatedCount can be incremented twice for the same memory — once at line 734 (per-memory truncation) and again here (total-budget truncation). This makes the debug log misleading (e.g. reporting truncated=3 when only 2 distinct lines were affected). Consider tracking truncated lines with a Set or a boolean flag per iteration instead.
There was a problem hiding this comment.
Resolved in c588558: truncatedCount now uses a per-iteration flag, so one emitted memory line is counted at most once even if both per-memory and total-budget truncation apply.
| if (perMemoryBounded.length > remainingChars) { | ||
| if (remainingChars >= MIN_TRUNCATED_RECALL_LINE_CHARS) { | ||
| const totalBounded = truncateRecallLine(perMemoryBounded, remainingChars); | ||
| budgeted.push(totalBounded); | ||
| usedChars += separatorChars + totalBounded.length; | ||
| if (totalBounded !== perMemoryBounded) truncatedCount++; | ||
| } else { | ||
| droppedCount++; | ||
| } | ||
| droppedCount += lines.length - i - 1; | ||
| break; |
There was a problem hiding this comment.
The droppedCount accumulation is split across the if/else branch and the line after it, making the logic harder to follow. Suggestion:
if (perMemoryBounded.length > remainingChars) {
const canFit = remainingChars >= MIN_TRUNCATED_RECALL_LINE_CHARS;
if (canFit) {
budgeted.push(truncateRecallLine(perMemoryBounded, remainingChars));
}
droppedCount += lines.length - i - (canFit ? 1 : 0);
break;
}Single accumulation point, same semantics, easier to reason about.
There was a problem hiding this comment.
Resolved in c588558: collapsed dropped-count handling to a single accumulation point in the total-budget overflow branch.
|
LGTM. Thanks for your contribution! By the way, there’s another issue #3 related to L1 recall. If you’re interested, feel free to take a look. |
Summary
recall.maxCharsPerMemoryandrecall.maxTotalRecallCharswith defaults of0, which do not alter existing behavior. Users can opt in by setting positive values to cap injected memory context.<relevant-memories>injection, preserving score order while truncating oversized entries and dropping overflow.openclaw.plugin.json.Test Plan
npm testnpm run buildnpm pack --dry-run --jsongit diff --checkNotes
npx tsc --noEmit ...still fails on currentupstream/maindue unrelated existing type issues in commander/OpenAI settings/offload strict initialization; this PR does not touch those areas.